feat(hash): Support hash field expiration #2402

jjz921024 · 2024-07-11T12:26:09Z

This pr want to close #2269

Encoding
To implement hash expiration by field level, we encode the expiration time into the first 8 bytes of value.
For example:
```
                     +-------------------------+
key|version|field => | expire (8 byte) | value |
                     +-------------------------+
```
And we use the second bit of the flags in metadata to indicate whether encode the expiration into value.
If all the fields in the hash have no encode expiration time, the flags will be 1 0 0 0 | 0 0 1 0.
If any field in the hash is encoded into the expiration time, the flags will be 1 1 0 0 | 0 0 1 0.
Flags conversion
For the HEXPIRE command, it will set an expiration on one or more fields.
If we execute hexpire command on a hash key that has not been set to expire, this will set the secode bit of flags and encode expiration for all fields (fields that not appear in the command will be set to 0)
Add commands
According to https://redis.io/docs/latest/commands add a series of related commands:
- HEXPIRE , HPEXPIRE , HEXPIREAT , HPEXPIREAT
- HEXPIRETIME , HPEXPIRETIME , HTTL , HPTTL
- HPERSIST

PragmaTwice · 2024-07-11T12:40:35Z

And we use the second bit of the flags in metadata to indicate whether encode the expiration into value.

As we specified in https://kvrocks.apache.org/community/data-structure-on-rocksdb#encoding-version-and-data-type, the first and second (and maybe more bits from the MSB) of flags is used to represent the "encoding version". flags 1 1 0 0 | 0 0 1 0 indicates that the encoding version is 3.

So it should not be used for field expiration. (Also, not all data types need the field expiration feature.)

Instead, you can add a new enum value of data type, or add a new field in the metadata of hash.

git-hulk · 2024-07-11T14:59:33Z

@jjz921024 Thanks for your contributions. As @PragmaTwice mentioned, we cannot put the expired flag in the version field and we also need to adopt the SubkeyFilter in compact_filter.cc to recycle expired subkeys.

jjz921024 · 2024-07-12T08:33:08Z

we also need to adopt the SubkeyFilter in compact_filter.cc to recycle expired subkeys.

@git-hulk Thanks for you reminding. I have added this feature in latest commit.

About encoding, my previous thought was to use each bit for a different meaning, rather than as an incremental version.
For example:

For all type, the first bit indicate use 64bit or 32bit encoding
For hash type, the second bit of the flags to indicate whether encode the expiration into value (not affect for other types)

I see this issues #2292. Maybe we should finish it first?

PragmaTwice · 2024-07-12T10:59:26Z

For all type, the first bit indicate use 64bit or 32bit encoding
For hash type, the second bit of the flags to indicate whether encode the expiration into value (not affect for other types)

No. It's too ad hoc and will introduce additional maintanance effort. (the encoding of HASH will be inconsistent with other types, which brings huge understanding, maintanance cost and additional code logic)

And also, it breaks our whole design of "encoding version", e.g. HASH cannot have encoding version larger than 1.

You can consider these options, as I mentioned before.

Instead, you can add a new enum value of data type, or add a new field in the metadata of hash.

-1 for this change if no further commits.

jjz921024 · 2024-07-12T14:44:18Z

@PragmaTwice Got it, i will change it.

src/commands/cmd_hash.cc

src/types/redis_hash.cc

src/storage/redis_db.cc

src/storage/redis_metadata.h

src/storage/redis_db.cc

torwig · 2024-07-14T09:53:42Z

@ jjz921024 Please let me know if I'm wrong. If the data structure contains at least 1 key that can be expired, we set a special bit to true and later check it.

src/types/redis_hash.cc

src/types/redis_hash.h

torwig · 2024-07-14T10:40:58Z

src/types/redis_hash.h


 private:
  rocksdb::Status GetMetadata(Database::GetOptions get_options, const Slice &ns_key, HashMetadata *metadata);
+  static rocksdb::Status decodeFieldValue(const HashMetadata &metadata, std::string *value, uint64_t &expire);
+  static rocksdb::Status encodeValueExpire(std::string *value, uint64_t expire);
+  static bool isMeetCondition(HashFieldExpireType type, uint64_t new_expire, uint64_t old_expire);


Can we change the name of this function to describe more clearly what it checks? What question does it answer?

Agree with what @torwig mentioned, for:

decodeFieldAndTTL should be separated into two methods: decodeExpireFromValue and checkIfFieldExpired, and encodeFieldAndTTL can rename to encodeExpireToValue. @torwig What do you think?

jjz921024 · 2024-07-15T14:28:02Z

@ jjz921024 Please let me know if I'm wrong. If the data structure contains at least 1 key that can be expired, we set a special bit to true and later check it.

Yes. Due to the expiration time is encoded in the field, the time complexity of some commands changes from O(1) to O(n), such as type and hlen. If we can check this flag before execution, then the some commands complexity will remains the same when
this feature is not used

jjz921024 · 2024-07-16T02:35:36Z

For all type, the first bit indicate use 64bit or 32bit encoding
For hash type, the second bit of the flags to indicate whether encode the expiration into value (not affect for other types)

No. It's too ad hoc and will introduce additional maintanance effort. (the encoding of HASH will be inconsistent with other types, which brings huge understanding, maintanance cost and additional code logic)

And also, it breaks our whole design of "encoding version", e.g. HASH cannot have encoding version larger than 1.

You can consider these options, as I mentioned before.

Instead, you can add a new enum value of data type, or add a new field in the metadata of hash.

-1 for this change if no further commits.

@PragmaTwice Hi, I fix it about encoding version. and add a new field in the metadata of hash to indicate whether the expiration time of field is encoded. please review. thank

src/storage/redis_metadata.h

src/storage/redis_db.cc

src/storage/redis_metadata.h

src/storage/redis_metadata.cc

src/types/redis_hash.cc

src/commands/cmd_hash.cc

src/types/redis_hash.cc

src/storage/redis_metadata.h

git-hulk · 2024-07-18T05:04:39Z

src/types/redis_hash.h


 private:
  rocksdb::Status GetMetadata(Database::GetOptions get_options, const Slice &ns_key, HashMetadata *metadata);
+  static rocksdb::Status decodeFieldValue(const HashMetadata &metadata, std::string *value, uint64_t &expire);
+  static rocksdb::Status encodeValueExpire(std::string *value, uint64_t expire);
+  static bool isMeetCondition(HashFieldExpireType type, uint64_t new_expire, uint64_t old_expire);


Agree with what @torwig mentioned, for:

decodeFieldAndTTL should be separated into two methods: decodeExpireFromValue and checkIfFieldExpired, and encodeFieldAndTTL can rename to encodeExpireToValue. @torwig What do you think?

src/storage/compact_filter.cc

src/types/redis_hash.cc

jjz921024 · 2024-07-19T02:35:36Z

@git-hulk Hi, I had renamed the decodeFieldAndTTL to decodeExpireFromValue, encodeFieldAndTTL to encodeExpireToValue. And rewrite the IsFieldExpired() to make it clearer. please review.

jjz921024 · 2024-07-27T07:31:19Z

Modification:

In hlen command, count the number of fields at hash expiration enable, replace the previous method to avoid loading all of fields into memory.
For most Generic type commands (eg: ttl, type, copy, expire, scan...). If the type is hash, we still need to check each fields. If all of fields was expired, the GetMetadata should return not found.
In del command, if delete a hash object that all of fields expired, so this hash object should be treated as empty and should not affect the deleted_cnt.

src/commands/cmd_hash.cc

src/storage/redis_metadata.cc

src/types/redis_hash.cc

PragmaTwice

As I mentioned before, we can add a config option (e.g. hash-field-expiration yes/no) for users to decide if they want to enable field expiration.

Also our testing can benefit from this option since we can cover the old hash encoding.

jjz921024 · 2024-07-29T11:36:21Z

As I mentioned before, we can add a config option (e.g. hash-field-expiration yes/no) for users to decide if they want to enable field expiration.

Also our testing can benefit from this option since we can cover the old hash encoding.

@PragmaTwice Thank for you tip, I have finished this feature. please review.

src/storage/redis_db.cc

tests/gocase/unit/type/hash/hash_test.go

PragmaTwice

See comments : )

jjz921024 · 2024-08-10T04:44:31Z

@git-hulk @PragmaTwice @torwig Sorry to bother your. I have changed the code as review required. May I ask your review it again? I think this is a very useful feature and hope it can be merge to kvrocks.
If there are any changes, please let me know. Thank.

jjz921024 · 2024-08-11T08:24:34Z

I had fixed the gofmt error and passed all CI in my fork repo.

https://github.com/jjz921024/kvrocks/actions/runs/10337719500

torwig · 2024-08-11T09:03:45Z

src/config/config.h

@@ -167,6 +167,9 @@ struct Config {
  int json_max_nesting_depth = 1024;
  JsonStorageFormat json_storage_format = JsonStorageFormat::JSON;

+  // hash


The variable name is self-explanatory so you can remove this comment. Otherwise, it would be best if you elaborated on the goal of this variable/option.

Yes. I had polished the comment of here

PragmaTwice · 2024-08-11T10:09:06Z

Seems we need to solve the conflict first?

jjz921024 · 2024-08-12T08:38:14Z

Seems we need to solve the conflict first?

rebase this pr on the latest commit of unstable branch.

jjz921024 · 2024-08-13T01:28:45Z

Seems there are failure in CI. I will check it up

src/storage/redis_metadata.cc

sonarcloud · 2024-08-13T09:25:52Z

Quality Gate passed

Issues
40 New issues
0 Accepted issues

Measures
0 Security Hotspots
60.9% Coverage on New Code
4.8% Duplication on New Code

See analysis details on SonarCloud

git-hulk · 2024-08-15T07:41:23Z

@jjz921024 I will take another pass in a few days, thanks for your contribution.

jjz921024 force-pushed the hfe branch from c16d864 to 7854e04 Compare July 11, 2024 12:31

jjz921024 changed the title ~~Support hash field expiration~~ feat(hash): Support hash field expiration Jul 11, 2024

jjz921024 marked this pull request as ready for review July 11, 2024 12:36

jjz921024 force-pushed the hfe branch from 7854e04 to dfdb385 Compare July 12, 2024 08:33